Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align with core/std on overflowing_sh* #430

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Align with core/std on overflowing_sh* #430

merged 3 commits into from
Dec 15, 2023

Conversation

tarcieri
Copy link
Member

Changes all shift functions which return an overflow flag (as a Choice or ConstChoice) to use the overflowing_sh* name prefix, which aligns with similar APIs in core/std.

In their place, adds new Uint::{shl, shr} functions which provide the trait-like behavior (i.e. panic on overflow) but work in const fn contexts (and can now panic at compile time on overflow).

Changes all shift functions which return an overflow flag (as a `Choice`
or `ConstChoice`) to use the `overflowing_sh*` name prefix, which aligns
with similar APIs in `core`/`std`.

In their place, adds new `Uint::{shl, shr}` functions which provide the
trait-like behavior (i.e. panic on overflow) but work in `const fn`
contexts (and can now panic at compile time on overflow).
@tarcieri tarcieri requested a review from fjarri December 14, 2023 21:28
@tarcieri
Copy link
Member Author

Note: the name changes were done using automated refactoring with IntelliJ, so I still haven't carefully gone through and determined what locations which are using overflowing_sh* should be changed to just sh*, but I tried to get the obvious ones.

//! pub const MODULUS_SHR1: U256 = MODULUS.shr(1).0;
//! pub const MODULUS_SHR1: U256 = MODULUS.shr(1);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like an ergonomics improvement

src/uint/shl.rs Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member Author

tarcieri commented Dec 14, 2023

There are a few API gaps which should probably be filled before merging if we're happy this, like every Uint::overflowing_sh* method should have a corresponding Uint::sh* method which is const fn.

BoxedUint doesn't need that because the methods are callable as traits in the prelude and it can't be const fn.

@@ -5,9 +5,22 @@ use core::ops::{Shl, ShlAssign};

impl<const LIMBS: usize> Uint<LIMBS> {
/// Computes `self << shift`.
///
/// Panics if `shift >= Self::BITS`.
pub const fn shl(&self, shift: u32) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need these methods? Primitive types don't have them.

Copy link
Member Author

@tarcieri tarcieri Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's what makes this possible, i.e. using them in a const fn context (and even better, doing compile-time overflow checking).

Primitive types are a bit magical in that arithmetic operators like << and >> work in const contexts even though for all other types they'd thunk through the Shl/Shr traits which won't be able to do that until const impl is available.

@fjarri
Copy link
Contributor

fjarri commented Dec 14, 2023

LGTM, except that I think _with_carry() methods should keep the names.

@tarcieri
Copy link
Member Author

Will fix. Thanks!

@tarcieri tarcieri merged commit d5e00ba into master Dec 15, 2023
16 checks passed
@tarcieri tarcieri deleted the overflowing-shift branch December 15, 2023 00:37
@tarcieri tarcieri mentioned this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants